Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multiple issues with MSVC Windows build #178

Merged
merged 3 commits into from
Jan 3, 2024
Merged

Conversation

elasota
Copy link
Contributor

@elasota elasota commented Dec 26, 2023

This fixes several problems with MSVC build and Windows:

  • Adds VS things to .gitignore
  • Fixes path processing not handling backslash separators
  • Fixes extract failure with absolute paths due to the directory existence check attempting to call stat on the drive, which doesn't work, then attempting to call _mkdir for the drive, which also doesn't work
  • Fixes libunshield not exporting any symbols in DLL configuration

lib/helper.c Outdated Show resolved Hide resolved
@twogood twogood self-assigned this Jan 3, 2024
@twogood twogood merged commit bfba780 into twogood:main Jan 3, 2024
6 checks passed
@twogood
Copy link
Owner

twogood commented Jan 3, 2024

Thank you @elasota and Happy New Year!

@@ -25,6 +25,7 @@ if(BUILD_STATIC)
add_library(libunshield STATIC ${LIBUNSHIELD_HEADERS} ${LIBUNSHIELD_SOURCES})
else()
add_library(libunshield SHARED ${LIBUNSHIELD_HEADERS} ${LIBUNSHIELD_SOURCES})
add_compile_definitions(LIBUNSHIELD_DYNAMIC_LIBRARY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this isn't a private target definition it might leak and cause clients to define it and use dllexport

@@ -16,6 +16,12 @@
#ifdef __cplusplus
extern "C" {
#endif

#if defined(_MSC_VER) && defined(LIBUNSHIELD_DYNAMIC_LIBRARY)
#define UNSHIELD_DLLEXPORT __declspec(dllexport)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You get better optimization if you define it as dllimport for clients

@twogood
Copy link
Owner

twogood commented Jan 3, 2024

Thanks @bwrsandman , looks like we might need a new PR to improve this one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants